Add server.request.body.filenames support for Jetty#10988
Open
Add server.request.body.filenames support for Jetty#10988
Conversation
jandro996
added
comp: asm waf
BenchmarksStartupParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 59 metrics, 11 unstable metrics.
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.61.0-SNAPSHOT~af78ea4bbd, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.067 s) : 0, 1066654
Total [baseline] (11.155 s) : 0, 11154898
Agent [candidate] (1.056 s) : 0, 1055942
Total [candidate] (11.005 s) : 0, 11005205
section appsec
Agent [baseline] (1.247 s) : 0, 1246769
Total [baseline] (11.147 s) : 0, 11147015
Agent [candidate] (1.254 s) : 0, 1254244
Total [candidate] (11.036 s) : 0, 11035826
section iast
Agent [baseline] (1.224 s) : 0, 1224369
Total [baseline] (11.235 s) : 0, 11234507
Agent [candidate] (1.245 s) : 0, 1244520
Total [candidate] (11.29 s) : 0, 11289607
section profiling
Agent [baseline] (1.186 s) : 0, 1186260
Total [baseline] (10.994 s) : 0, 10994154
Agent [candidate] (1.189 s) : 0, 1188736
Total [candidate] (10.969 s) : 0, 10968590
gantt
title petclinic - break down per module: candidate=1.61.0-SNAPSHOT~af78ea4bbd, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.236 ms) : 0, 1236
crashtracking [candidate] (1.219 ms) : 0, 1219
BytebuddyAgent [baseline] (638.38 ms) : 0, 638380
BytebuddyAgent [candidate] (632.595 ms) : 0, 632595
AgentMeter [baseline] (30.065 ms) : 0, 30065
AgentMeter [candidate] (29.51 ms) : 0, 29510
GlobalTracer [baseline] (251.849 ms) : 0, 251849
GlobalTracer [candidate] (248.321 ms) : 0, 248321
AppSec [baseline] (32.891 ms) : 0, 32891
AppSec [candidate] (32.403 ms) : 0, 32403
Debugger [baseline] (60.61 ms) : 0, 60610
Debugger [candidate] (59.827 ms) : 0, 59827
Remote Config [baseline] (593.885 µs) : 0, 594
Remote Config [candidate] (593.141 µs) : 0, 593
Telemetry [baseline] (8.084 ms) : 0, 8084
Telemetry [candidate] (9.578 ms) : 0, 9578
Flare Poller [baseline] (6.643 ms) : 0, 6643
Flare Poller [candidate] (5.862 ms) : 0, 5862
section appsec
crashtracking [baseline] (1.232 ms) : 0, 1232
crashtracking [candidate] (1.224 ms) : 0, 1224
BytebuddyAgent [baseline] (660.809 ms) : 0, 660809
BytebuddyAgent [candidate] (667.39 ms) : 0, 667390
AgentMeter [baseline] (12.224 ms) : 0, 12224
AgentMeter [candidate] (12.201 ms) : 0, 12201
GlobalTracer [baseline] (248.196 ms) : 0, 248196
GlobalTracer [candidate] (248.808 ms) : 0, 248808
IAST [baseline] (24.482 ms) : 0, 24482
IAST [candidate] (24.527 ms) : 0, 24527
AppSec [baseline] (184.859 ms) : 0, 184859
AppSec [candidate] (185.003 ms) : 0, 185003
Debugger [baseline] (65.947 ms) : 0, 65947
Debugger [candidate] (65.99 ms) : 0, 65990
Remote Config [baseline] (618.998 µs) : 0, 619
Remote Config [candidate] (625.557 µs) : 0, 626
Telemetry [baseline] (8.522 ms) : 0, 8522
Telemetry [candidate] (8.511 ms) : 0, 8511
Flare Poller [baseline] (3.591 ms) : 0, 3591
Flare Poller [candidate] (3.532 ms) : 0, 3532
section iast
crashtracking [baseline] (1.236 ms) : 0, 1236
crashtracking [candidate] (1.235 ms) : 0, 1235
BytebuddyAgent [baseline] (800.491 ms) : 0, 800491
BytebuddyAgent [candidate] (815.535 ms) : 0, 815535
AgentMeter [baseline] (11.598 ms) : 0, 11598
AgentMeter [candidate] (11.821 ms) : 0, 11821
GlobalTracer [baseline] (239.412 ms) : 0, 239412
GlobalTracer [candidate] (241.987 ms) : 0, 241987
IAST [baseline] (26.397 ms) : 0, 26397
IAST [candidate] (26.209 ms) : 0, 26209
AppSec [baseline] (30.469 ms) : 0, 30469
AppSec [candidate] (32.459 ms) : 0, 32459
Debugger [baseline] (65.141 ms) : 0, 65141
Debugger [candidate] (65.11 ms) : 0, 65110
Remote Config [baseline] (532.88 µs) : 0, 533
Remote Config [candidate] (558.777 µs) : 0, 559
Telemetry [baseline] (9.41 ms) : 0, 9410
Telemetry [candidate] (9.412 ms) : 0, 9412
Flare Poller [baseline] (3.595 ms) : 0, 3595
Flare Poller [candidate] (3.717 ms) : 0, 3717
section profiling
crashtracking [baseline] (1.172 ms) : 0, 1172
crashtracking [candidate] (1.197 ms) : 0, 1197
BytebuddyAgent [baseline] (691.609 ms) : 0, 691609
BytebuddyAgent [candidate] (694.375 ms) : 0, 694375
AgentMeter [baseline] (9.334 ms) : 0, 9334
AgentMeter [candidate] (8.934 ms) : 0, 8934
GlobalTracer [baseline] (208.334 ms) : 0, 208334
GlobalTracer [candidate] (207.782 ms) : 0, 207782
AppSec [baseline] (32.812 ms) : 0, 32812
AppSec [candidate] (32.795 ms) : 0, 32795
Debugger [baseline] (65.671 ms) : 0, 65671
Debugger [candidate] (65.951 ms) : 0, 65951
Remote Config [baseline] (593.967 µs) : 0, 594
Remote Config [candidate] (575.046 µs) : 0, 575
Telemetry [baseline] (7.804 ms) : 0, 7804
Telemetry [candidate] (7.844 ms) : 0, 7844
Flare Poller [baseline] (3.528 ms) : 0, 3528
Flare Poller [candidate] (3.612 ms) : 0, 3612
ProfilingAgent [baseline] (93.869 ms) : 0, 93869
ProfilingAgent [candidate] (94.159 ms) : 0, 94159
Profiling [baseline] (94.417 ms) : 0, 94417
Profiling [candidate] (94.731 ms) : 0, 94731
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.61.0-SNAPSHOT~af78ea4bbd, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.055 s) : 0, 1055472
Total [baseline] (8.794 s) : 0, 8793860
Agent [candidate] (1.06 s) : 0, 1059916
Total [candidate] (8.846 s) : 0, 8845623
section iast
Agent [baseline] (1.23 s) : 0, 1230279
Total [baseline] (9.533 s) : 0, 9533094
Agent [candidate] (1.223 s) : 0, 1223268
Total [candidate] (9.514 s) : 0, 9513576
gantt
title insecure-bank - break down per module: candidate=1.61.0-SNAPSHOT~af78ea4bbd, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.225 ms) : 0, 1225
crashtracking [candidate] (1.245 ms) : 0, 1245
BytebuddyAgent [baseline] (631.942 ms) : 0, 631942
BytebuddyAgent [candidate] (637.741 ms) : 0, 637741
AgentMeter [baseline] (29.559 ms) : 0, 29559
AgentMeter [candidate] (29.677 ms) : 0, 29677
GlobalTracer [baseline] (248.188 ms) : 0, 248188
GlobalTracer [candidate] (249.861 ms) : 0, 249861
AppSec [baseline] (32.401 ms) : 0, 32401
AppSec [candidate] (32.795 ms) : 0, 32795
Debugger [baseline] (59.18 ms) : 0, 59180
Debugger [candidate] (59.43 ms) : 0, 59430
Remote Config [baseline] (601.002 µs) : 0, 601
Remote Config [candidate] (597.932 µs) : 0, 598
Telemetry [baseline] (9.549 ms) : 0, 9549
Telemetry [candidate] (8.826 ms) : 0, 8826
Flare Poller [baseline] (6.698 ms) : 0, 6698
Flare Poller [candidate] (3.526 ms) : 0, 3526
section iast
crashtracking [baseline] (1.23 ms) : 0, 1230
crashtracking [candidate] (1.228 ms) : 0, 1228
BytebuddyAgent [baseline] (807.203 ms) : 0, 807203
BytebuddyAgent [candidate] (800.738 ms) : 0, 800738
AgentMeter [baseline] (11.689 ms) : 0, 11689
AgentMeter [candidate] (11.587 ms) : 0, 11587
GlobalTracer [baseline] (239.588 ms) : 0, 239588
GlobalTracer [candidate] (239.04 ms) : 0, 239040
IAST [baseline] (25.759 ms) : 0, 25759
IAST [candidate] (25.746 ms) : 0, 25746
AppSec [baseline] (32.048 ms) : 0, 32048
AppSec [candidate] (32.124 ms) : 0, 32124
Debugger [baseline] (63.32 ms) : 0, 63320
Debugger [candidate] (63.215 ms) : 0, 63215
Remote Config [baseline] (534.759 µs) : 0, 535
Remote Config [candidate] (546.134 µs) : 0, 546
Telemetry [baseline] (9.271 ms) : 0, 9271
Telemetry [candidate] (9.425 ms) : 0, 9425
Flare Poller [baseline] (3.533 ms) : 0, 3533
Flare Poller [candidate] (3.599 ms) : 0, 3599
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 5 performance regressions! Performance is the same for 13 metrics, 17 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~af78ea4bbd, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section baseline
no_agent (18.36 ms) : 18178, 18543
. : milestone, 18360,
appsec (18.634 ms) : 18446, 18822
. : milestone, 18634,
code_origins (17.992 ms) : 17815, 18169
. : milestone, 17992,
iast (17.992 ms) : 17813, 18171
. : milestone, 17992,
profiling (18.187 ms) : 18005, 18369
. : milestone, 18187,
tracing (17.965 ms) : 17786, 18144
. : milestone, 17965,
section candidate
no_agent (18.018 ms) : 17838, 18198
. : milestone, 18018,
appsec (19.239 ms) : 19046, 19433
. : milestone, 19239,
code_origins (18.587 ms) : 18401, 18772
. : milestone, 18587,
iast (18.04 ms) : 17862, 18217
. : milestone, 18040,
profiling (19.539 ms) : 19340, 19738
. : milestone, 19539,
tracing (18.014 ms) : 17835, 18193
. : milestone, 18014,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~af78ea4bbd, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section baseline
no_agent (1.277 ms) : 1265, 1289
. : milestone, 1277,
iast (3.384 ms) : 3340, 3428
. : milestone, 3384,
iast_FULL (5.984 ms) : 5923, 6045
. : milestone, 5984,
iast_GLOBAL (3.698 ms) : 3639, 3756
. : milestone, 3698,
profiling (2.057 ms) : 2040, 2074
. : milestone, 2057,
tracing (1.894 ms) : 1878, 1910
. : milestone, 1894,
section candidate
no_agent (1.271 ms) : 1259, 1284
. : milestone, 1271,
iast (3.224 ms) : 3182, 3267
. : milestone, 3224,
iast_FULL (6.095 ms) : 6032, 6158
. : milestone, 6095,
iast_GLOBAL (3.632 ms) : 3569, 3696
. : milestone, 3632,
profiling (2.32 ms) : 2298, 2342
. : milestone, 2320,
tracing (1.919 ms) : 1903, 1935
. : milestone, 1919,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 2 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~af78ea4bbd, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section baseline
no_agent (15.254 s) : 15254000, 15254000
. : milestone, 15254000,
appsec (15.305 s) : 15305000, 15305000
. : milestone, 15305000,
iast (18.353 s) : 18353000, 18353000
. : milestone, 18353000,
iast_GLOBAL (18.207 s) : 18207000, 18207000
. : milestone, 18207000,
profiling (15.405 s) : 15405000, 15405000
. : milestone, 15405000,
tracing (14.79 s) : 14790000, 14790000
. : milestone, 14790000,
section candidate
no_agent (14.954 s) : 14954000, 14954000
. : milestone, 14954000,
appsec (14.804 s) : 14804000, 14804000
. : milestone, 14804000,
iast (18.212 s) : 18212000, 18212000
. : milestone, 18212000,
iast_GLOBAL (18.074 s) : 18074000, 18074000
. : milestone, 18074000,
profiling (14.856 s) : 14856000, 14856000
. : milestone, 14856000,
tracing (14.994 s) : 14994000, 14994000
. : milestone, 14994000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~af78ea4bbd, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section baseline
no_agent (1.482 ms) : 1470, 1493
. : milestone, 1482,
appsec (3.837 ms) : 3614, 4060
. : milestone, 3837,
iast (2.276 ms) : 2207, 2345
. : milestone, 2276,
iast_GLOBAL (2.314 ms) : 2244, 2384
. : milestone, 2314,
profiling (2.117 ms) : 2062, 2173
. : milestone, 2117,
tracing (2.081 ms) : 2027, 2134
. : milestone, 2081,
section candidate
no_agent (1.482 ms) : 1471, 1494
. : milestone, 1482,
appsec (3.855 ms) : 3631, 4078
. : milestone, 3855,
iast (2.269 ms) : 2199, 2338
. : milestone, 2269,
iast_GLOBAL (2.319 ms) : 2250, 2389
. : milestone, 2319,
profiling (2.518 ms) : 2352, 2685
. : milestone, 2518,
tracing (2.109 ms) : 2054, 2164
. : milestone, 2109,
|
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
from
e3d4073 to
e2d5ed0
Compare
Add GetFilenamesAdvice to all three Jetty AppSec modules to collect uploaded file names from multipart requests and fire the requestFilesFilenames() IG callback: - jetty-appsec-8.1.3: intercepts getParts() return value; includes Content-Disposition header fallback for Servlet 3.0 (Jetty 9.0) where getSubmittedFileName() is not available - jetty-appsec-9.2: intercepts no-arg getParts() for Servlet 3.1+ - jetty-appsec-9.3: same, applies to Jetty 9.3, 10, 11 Enable testBodyFilenames() in Jetty 9.x, 10 and 11 server tests.
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
from
f2998c3 to
629f074
Compare
| } | ||
| } | ||
| // Fallback: parse filename from Content-Disposition header (Servlet 3.0) | ||
| if (name == null) { |
There was a problem hiding this comment.
Shouldn't this be outside of the main parts loop?
Member
Author
There was a problem hiding this comment.
Fixed. Restructured into two separate loops chosen once before iteration: if getSubmittedFileName != null (Servlet 3.1+) iterate using that method; otherwise iterate parsing the Content-Disposition header (Servlet 3.0 fallback). No per-part branching inside the loop.
| transformer.applyAdvice( | ||
| named("extractContentParameters").and(takesArguments(0)).or(named("getParts")), | ||
| getClass().getName() + "$ExtractContentParametersAdvice"); | ||
| transformer.applyAdvice(named("getParts"), getClass().getName() + "$GetFilenamesAdvice"); |
Member
Author
There was a problem hiding this comment.
Fixed. GetFilenamesAdvice now has a call-depth guard (CallDepthThreadLocalMap with Collection.class) to avoid double-firing when getParts() internally calls getParts(MultiMap)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c732823549
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
2 times, most recently
from
ecf65c5 to
eae08aa
Compare
…MultiMap) path - jetty-appsec-9.3: add call-depth guard (Collection.class) to GetFilenamesAdvice to prevent double callback invocation when getParts() calls getParts(MultiMap) internally - jetty-appsec-9.2: extend GetFilenamesAdvice matcher to all getParts overloads (not just no-arg) to cover getParameter*()/getParameterMap() code paths, guarded with same call-depth mechanism to avoid double-firing
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
2 times, most recently
from
3ab9ff7 to
77ec572
Compare
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
from
2e72584 to
d37e03e
Compare
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
from
d37e03e to
d8a92f8
Compare
- Add BODY_MULTIPART_REPEATED case to TestServlet3 (javax) so Jetty 9.x/10.x test modules can exercise the repeated getParts() scenario - Enable testBodyFilenamesCalledOnce() for Jetty 9.0, 9.0.4, 9.3, 9.4.21, and 10.0
…vice path - New BODY_MULTIPART_COMBINED endpoint: calls getParameterMap() first (triggers GetFilenamesFromMultiPartAdvice via extractContentParameters -> getParts(MultiMap)), then getParts() explicitly (GetFilenamesAdvice must not double-fire since _contentParameters is already set) - New test 'file upload filenames called once via parameter map' verifies the callback fires exactly once across both advice paths - Enabled in Jetty 9.0, 9.0.4, 9.3, 9.4.21, 10.0 and 11.0
…eplaces _contentParameters as the getParts() cache In Jetty 9.3, getParts(MultiMap) sets _contentParameters, so the map==null guard prevents re-firing on repeated getParts() calls. In Jetty 9.4+, getParts() delegates to getParts(null) and caches the result in _multiParts instead, leaving _contentParameters null on every call. Add _multiParts==null as an additional guard (optional=true handles Jetty 9.3 where the field does not exist).
…utStream - jetty-appsec-9.4: extend muzzle to also cover Jetty 10.0.0–10.0.9, which were previously a gap. In those versions _multiParts is typed MultiPartFormInputStream (not MultiParts); the primary Reference spec matches 9.4.10+ and 10.0.10+, and an OrReference alternative matches 10.0.0–10.0.9. The GetFilenamesAdvice already uses typing=DYNAMIC so no advice changes are needed. - jetty-appsec-8.1.3 PartHelper: wrap part.getInputStream() in try-with-resources to avoid leaking file descriptors on file-backed multipart form fields.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f2e2b3998
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Splitting the header on ';' naively truncated filenames that contain semicolons inside a quoted value, e.g. filename="shell;evil.php" would produce "shell" instead of the full name. Replace the split() loop with a quote-aware state-machine parser that skips semicolons inside quoted strings and handles backslash-escaped characters. Add test cases for semicolons in filenames, escaped quotes, and filename appearing before other parameters.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ce4bf1084
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…c-11.0 Replace the JAKARTA_PART_REFERENCE classpath check with a _dispatcherType field descriptor check on Request.class bytecode, mirroring the approach already used by jetty-appsec-9.4. The classpath check passes on any Jetty 9.4/10 app that has jakarta.servlet-api as a dependency, causing double-instrumentation of extractContentParameters. The bytecode check is authoritative: in Jetty 11+ Request.class carries _dispatcherType as Ljakarta/servlet/DispatcherType;, while 9.4/10 carry the javax descriptor.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74948a75d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…uploads In Jetty 8.x, getPart(String name) calls _multiPartInputStream.getPart(String) directly without delegating to getParts(). Applications that retrieve only one file via getPart() without ever calling getParts() would have their filename event missed. Add GetPartAdvice to cover this path. The charset fix (AI comment 2) was investigated and is not applicable: HTML5 form submissions always use UTF-8 and browsers never include charset= on individual part Content-Type headers, so the existing hardcoded UTF-8 is correct.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1508fef49c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ilename tests in async suite 1. Add _multiPartInputStream == null guard to GetFilenamesAdvice.before() so that repeated getParts() calls on the same request (which Jetty caches) do not re-fire requestFilesFilenames/requestBodyProcessed WAF callbacks. The field is null before the first multipart parse and non-null on all subsequent cached calls, matching the pattern used in the 9.4/11.0 advice (_multiParts guard). 2. JettyAsyncHandlerTest already disabled testBodyFilenames() but neglected to disable testBodyFilenamesCalledOnce() and testBodyFilenamesCalledOnceCombined(), which are now enabled in the Jetty11Test parent. Override both to false in the async handler suite to prevent spurious test failures.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bece33644c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…elper filenameFromPart() was returning null for both 'no filename parameter' and 'filename=""', causing extractFormFields() to buffer the full body of file inputs submitted with no file chosen (filename=""). An empty <input type=file> is still a file part, not a form field. Return "" instead of null so that callers using != null correctly skip those parts without reading their content. Update tests to assert "" for empty-filename cases and add regression tests for extractFormFields/extractFilenames with empty-filename parts. Note: the second AI comment about getPart(String) double-firing was not implemented. The bytecode shows the internal call is to MultiPartInputStream.getParts() (not Request.getParts()), so GetFilenamesAdvice (which instruments Request.getParts()) is never triggered during a getPart() call. There is no double-firing.
The jetty-appsec-9.4 early_10_series muzzle pass covers [10.0.0, 10.0.10) where _multiParts is MultiPartFormInputStream (before the type changed to MultiParts in 10.0.10). The existing test suite used 10.0.10, leaving the early versions untested. Add earlyDep10ForkedTest (Jetty 10.0.9) to exercise the OR-muzzle reference and verify that multipart filename events fire correctly on the old field type. All three filename tests pass: filenames, filenames-called-once, filenames-combined.
…M API constant Move duplicated IG-callback + blocking-commit blocks out of advice into helper methods (MultipartHelper.fireFilenamesEvent / PartHelper.fireBodyProcessedEvent + fireFilenamesEvent) that return BlockingException|null, exploiting @Advice.Thrown(readOnly=false) assignment behaviour. Also replace hardcoded Opcodes.ASM9 with OpenedClassReader.ASM_API in RequestGetPartsInstrumentation and fix extractFormFields to use computeIfAbsent instead of get+put.
Member
Author
|
@code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 116d7a270d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…() delegates to getParts() In Jetty 9.0/9.1 (covered by jetty-appsec-8.1.3's [8.1.3,9.2.0.RC0) range), Request.getPart(String) delegates to getParts() internally. Without this fix, GetFilenamesAdvice fires for the full collection (via the internal getParts() call) and then GetPartAdvice fires again for the returned singleton, because the two advice classes use independent call-depth keys and cannot see each other. Fix: peek at the Part.class call depth in GetFilenamesAdvice.before(). If > 1, GetPartAdvice is active on the stack and will handle the event; skip to avoid double-firing. In Jetty 8.x (where getPart() does not delegate), the peek is always 1, so GetFilenamesAdvice continues to fire normally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
Instruments Jetty's multipart request handling to fire both
requestFilesFilenamesandrequestBodyProcessedAppSec gateway events, enabling WAF rules that act on uploaded filenames and multipart form-field values.Jetty parses multipart bodies through two code paths depending on how the application accesses the request:
getParameterMap()→extractContentParameters()→ internalgetParts(MultiMap)getParts()/getPart(String)call from user codeBoth paths are instrumented where they exist. Guards on internal state fields (
_contentParameters,_multiParts,_multiPartInputStream) prevent double-firing when the same method is called more than once per request.Module coverage
jetty-appsec-7.0getParts()does not exist in Jetty 7.xjetty-appsec-8.1.3[8.1.3, 9.2.0.RC0)jetty-appsec-9.2[9.2, 9.3)requestBodyProcessed; no filename support needed at this rangejetty-appsec-9.3[9.3, 9.4.10)[9.3, 12). AddedGetFilenamesAdvice+GetFilenamesFromMultiPartAdvice. Double-fire guard uses_multiPartInputStream.jetty-appsec-9.4[9.4.10, 11.0)_multiParts: MultiParts(9.4.10+, 10.0.10+) and_multiParts: MultiPartFormInputStream(10.0.0–10.0.9). Bytecode discriminator:_dispatcherType: Ljavax/servlet/DispatcherType;.jetty-appsec-11.0[11.0, 12.0)_dispatcherType: Ljakarta/servlet/DispatcherType;inRequest.classbytecode — checking classpath presence ofjakarta.servletwas unreliable when bothjavaxandjakartawere present simultaneously.jetty-appsec-8.1.3 details
Replaced brittle ASM bytecode injection (
GetPartsMethodVisitor) with clean exit advice:GetFilenamesAdviceongetParts()— firesrequestBodyProcessed(form fields) +requestFilesFilenamesGetPartAdviceongetPart(String)— same, for single-part uploads that never callgetParts(); in Jetty 8.xgetPart(String)calls_multiPartInputStream.getPart()directly_multiPartInputStream == nullguard prevents re-firing on repeatedgetParts()callsRequest.classbytecode at agent startup, detecting thegetParameters()call insidegetParts()that is unique to Jetty 8.xPartHelper(replacingParameterCollector):filenameFromPart(): quote-awareContent-Dispositionparser;Part.getSubmittedFileName()is Servlet 3.1+ and unavailable hereextractFormFields(): reads form-field body values, skips file-upload partsfireBodyProcessedEvent()/fireFilenamesEvent(): encapsulate the full IG callback + blocking-commit logic, returningBlockingException|nullfor use with@Advice.Thrown(readOnly=false)Helpers and tests
PartHelper(8.1.3) andMultipartHelper(9.3/9.4/11.0) are unit-tested independently inPartHelperTest/MultipartHelperTestjetty-server-7.6,jetty-server-9.3,jetty-server-9.4.21,jetty-server-10.0,jetty-server-11.0cover both thegetParts()andgetParameterMap()code paths; a dedicatedJetty10EarlyDepForkedTestcovers Jetty 10.0.0–10.0.9Motivation
Part of APPSEC-61873 —
server.request.body.filenamesimplementation across server frameworks.Additional Notes
Bytecode discriminator rationale (9.4 vs 11.0): checking for
jakarta.servlet.http.Parton the classpath was unreliable when a Jetty 9.4/10 app ships bothjavaxandjakartaon the classpath (common with Spring Boot 3 migration tooling). The_dispatcherTypefield descriptor insideRequest.classbytecode is unambiguous regardless of classpath contents.GetFilenamesFromMultiPartAdvicevsGetFilenamesAdvice: in all 9.3/9.4/11.0 modules,extractContentParameters()sets_contentParametersbefore callinggetParts(MultiMap), so the_contentParameters != nullguard inGetFilenamesAdvicecorrectly prevents double-firing when thegetParameterMap()path is used.Part.getSubmittedFileName()availability: Servlet 3.1+ (Jetty 9.1+). For Jetty 8.x (Servlet 3.0),PartHelper.filenameFromPart()parses theContent-Dispositionheader manually with a quote-aware parser to handle filenames likefilename="shell;evil.php"correctly.Depends on #10973 (merged)
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APPSEC-61873
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.